Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[uss_qualifier] NET0460 evaluate display provider's flight data details endpoint performance #174

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Aug 18, 2023

Note that validating the performance of the DP for returning details requires us to actually generate some queries for that endpoint.

To this end we query details for each flight that is returned when checking the display_data endpoint.

@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch 3 times, most recently from d7861a9 to 56e72d5 Compare August 18, 2023 13:48
@Shastick Shastick changed the title NET0460 query observer's details endpoint to collect perf stats of DP details [uss_qualifier] NET0460 query observer's details endpoint to collect perf stats of DP details Aug 21, 2023
@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch 4 times, most recently from 8211dbf to 91448a4 Compare August 21, 2023 12:25
@Shastick Shastick changed the title [uss_qualifier] NET0460 query observer's details endpoint to collect perf stats of DP details [uss_qualifier] NET0460 evaluate display provider's flight data details endpoint performance Aug 21, 2023
@Shastick Shastick marked this pull request as ready for review August 21, 2023 13:02
@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch from 91448a4 to 8ead60a Compare August 21, 2023 13:03
@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch 2 times, most recently from 2548546 to faddc20 Compare August 24, 2023 07:21
@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch from faddc20 to 0fc2124 Compare August 31, 2023 09:38
@Shastick
Copy link
Contributor Author

Moving this to draft, as it will need some updates once #159 is ready

@Shastick Shastick marked this pull request as draft August 31, 2023 19:47
@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch 3 times, most recently from b184a0f to e46aa5c Compare September 1, 2023 15:10
@Shastick Shastick marked this pull request as ready for review September 6, 2023 10:28
@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch from e46aa5c to 8f96ba0 Compare September 6, 2023 10:28
@Shastick
Copy link
Contributor Author

Shastick commented Sep 6, 2023

Ready for another round of reviews (just updated on the latest master).

Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor issues to address before merge and some nits, otherwise LGTM.

@@ -65,6 +65,11 @@ def read_scope(self) -> str:
else:
raise ValueError("Unsupported RID version '{}'".format(self))

@property
def dss_read_isa_scope(self) -> str:
# Same regardless of ASTM RID version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add a TODO regarding Ben's comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI (not a prereq for merging this PR)

self.session,
"GET",
f"/display_data/{flight_id}",
scope=self.rid_version.dss_read_isa_scope,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: A few lines above the call GET /display_data?view=... uses read_scope. I'm aware of Ben's comment, but it may be better for the two calls to be consistent in the call they use. So maybe change the other call to use this, or put a TODO, or just put a constant with a TODO here and use it in both spots?

Copy link
Contributor Author

@Shastick Shastick Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I was pondering that: I was just wondering if the scopes should actually be different?

For v19 the read scope actually is the same as here (dss.read.identification_service_areas)

I'll give it a try and see if anything blows up

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope they should not be different, they both refer to the same definition: https://github.com/interuss/automated_testing_interfaces/blob/main/rid/v1/observation.yaml#L182

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed: both calls to /display_data and /display_data/{flight_id} use the same scope (which happens to be equal in value to an F3411-19 scope, but is semantically an InterUSS RID observation automated testing API scope)

@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch 4 times, most recently from 9bde64a to 938a876 Compare September 7, 2023 08:16
@Shastick
Copy link
Contributor Author

Shastick commented Sep 7, 2023

Cleaned up rid.py, added a magic string with the correct substitution in a TODO for after interuss/uas_standards#11 becomes available.

@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch 3 times, most recently from d82128c to dc0c9ac Compare September 7, 2023 19:06
@BenjaminPelletier
Copy link
Member

@mickmis : feel free to merge when ready & you think is appropriate

@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch 3 times, most recently from 632b4cf to c4d323d Compare September 11, 2023 14:51
@Shastick Shastick requested a review from mickmis September 11, 2023 15:09
@Shastick Shastick force-pushed the net-0460-dp-details-stats-checks branch from c4d323d to dcb0e9d Compare September 12, 2023 10:01
@mickmis mickmis merged commit 905765b into interuss:main Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants